Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

to_number accepts arguments of type json.Number #63

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

to_number accepts arguments of type json.Number #63

wants to merge 1 commit into from

Conversation

arl
Copy link

@arl arl commented Sep 30, 2021

to_number implementation fails to cast arguments of the standard type json.Number.
All numbers are converted to json.Number (a mere string) when UseNumber has been set on the json Decoder.

@arl
Copy link
Author

arl commented Nov 19, 2021

More context on the issue (and why the fix):
We sometimes need to set json.UseNumber on a json decoder. However, when we do so, go-jmespath fails to decode JSON numbers because jpfToNumber doesn't handle it.
The PR fixes that

@suntong
Copy link

suntong commented Dec 24, 2021

I think this PR is reasonable and should be merged.

@jamesls, what's your thought about it?

@springcomp
Copy link

@arl I’m working on expanding this implementation to bring it up on par to the latest version of JMESPath Community, I would be interested in taking this PR.

Is there a specific compliance test that would show the failure before this PR gets merged ?

@arl
Copy link
Author

arl commented Feb 22, 2023

Is there a specific compliance test that would show the failure before this PR gets merged ?

Hi @springcomp yes there should be a test.
I'll look into it, should be relatively easy, i'd just need to look at the code a bit since I've opened the PR it was 1.5 year ago.
Thanks for doing this btw

@kpacha kpacha mentioned this pull request Mar 9, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@arl @suntong @springcomp